feat(theme): establish a deliberate colour system - #519
Merged
Conversation
Colour was used as a signal in three unrelated ways with no single owner: category accents, stock availability, and personal tasted state. This consolidates all three into CategoryColorHelper and documents the model in the class doc, so colour choices have one place to live. - Remove getCategoryColor(), which had no call sites in lib/ at all. The "two category-colour paths that can drift" were never both live; the chips it was supposed to serve use colorScheme.primaryContainer. - Add getAvailabilityColor(), replacing the hex palette that was duplicated independently between the drinks list availability chip and the My Festival at-risk hint. Sold-out keeps tracking the theme error colour rather than a fixed hex. - Derive category accents from brightness so they adapt to dark mode: the hue is kept as-is for light, and lightness-lifted for dark. Deriving accents via ColorScheme.fromSeed(...).primary was measured and rejected: it desaturates the palette and collapses perry and apple juice to a summed-RGB distance of 2, destroying the at-a-glance category distinction the accent exists for. The lift keeps minimum separation at 63 against 72 today. A test pins that floor so a future derivation change cannot quietly reintroduce the collision. Light mode is byte-identical by construction, so only the six dark goldens changed; each was reviewed and shows an accent-colour change with no reflow. getTastedColor stays independent of getAvailabilityColor even though both are green today, since personal status and stock level are separate signals that may diverge. Fixes #469
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the app’s “signal” colors into a centralized, brightness-aware CategoryColorHelper, updates UI call sites to pass theme brightness explicitly, and replaces older widget tests with targeted unit tests for the new color system.
Changes:
- Updated
getAccentColorto be brightness-aware and migrated widgets/tests to pass theme brightness. - Consolidated availability-status colors into
CategoryColorHelper.getAvailabilityColorand removed duplicated hex logic from widgets. - Moved/remodeled color helper tests into
test/category_color_helper_test.dartwith stronger invariants (hue preservation, separation, etc.).
Reviewed changes
Copilot reviewed 10 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils_test.dart | Removes older getCategoryColor widget tests from the utils test suite. |
| test/screens/my_festival_screen_test.dart | Updates test to derive brightness from the pumped widget tree. |
| test/category_color_helper_test.dart | Adds focused tests for accent/availability/tasted colors and regression guards. |
| lib/utils/category_color_helper.dart | Implements brightness-aware category accents and centralized availability/tasted colors; removes getCategoryColor. |
| lib/widgets/style_hero_panel.dart | Passes brightness into getAccentColor. |
| lib/widgets/drink_hero_panel.dart | Passes brightness into getAccentColor. |
| lib/widgets/brewery_hero_panel.dart | Passes brightness into getAccentColor. |
| lib/widgets/drink_card.dart | Passes brightness into getAccentColor; uses getAvailabilityColor in _AvailabilityChip. |
| lib/screens/my_festival_screen.dart | Passes brightness into getAccentColor; uses getAvailabilityColor for at-risk hint. |
| lib/screens/drink_detail_screen.dart | Passes brightness into getAccentColor for similar drink cards. |
Comment on lines
485
to
498
| switch (status) { | ||
| case AvailabilityStatus.out: | ||
| color = theme.colorScheme.error; | ||
| atRisk = AvailabilityStatus.out; | ||
| icon = Icons.cancel; | ||
| label = 'Sold Out'; | ||
| case AvailabilityStatus.veryLow: | ||
| color = isDark ? const Color(0xFFFF7043) : const Color(0xFFBF360C); | ||
| atRisk = AvailabilityStatus.veryLow; | ||
| icon = Icons.warning_amber; | ||
| label = 'Nearly Gone'; | ||
| case AvailabilityStatus.low: | ||
| color = isDark ? const Color(0xFFFF9800) : const Color(0xFFEF6C00); | ||
| atRisk = AvailabilityStatus.low; | ||
| icon = Icons.warning; | ||
| label = 'Low'; | ||
| case AvailabilityStatus.plenty: |
Owner
Author
There was a problem hiding this comment.
Not acting on this one — Dart 3.0 removed the break requirement for non-empty switch cases, which no longer fall through. The surrounding code already relied on that: the case AvailabilityStatus.out: / veryLow / low bodies in this same method had no break before this PR either, and this project is on Dart >=3.10.
./bin/mise run check is green on this branch — analyzer clean and 1303 tests pass — so the code demonstrably compiles.
Generated by Claude Code
Comment on lines
+72
to
+77
| /// Derived from [brightness]: the fixed hue in light mode, a lightness-lifted | ||
| /// variant in dark mode so the edge reads against a dark surface. Falls back | ||
| /// to CBF navy for unknown categories. | ||
| static Color getAccentColor(String category, Brightness brightness) { | ||
| final hue = _categoryHues[category] ?? _fallbackHue; | ||
| return brightness == Brightness.dark ? _liftForDark(hue) : hue; |
Comment on lines
+112
to
+117
| static Color getAvailabilityColor( | ||
| AvailabilityStatus status, | ||
| ColorScheme colorScheme, | ||
| Brightness brightness, | ||
| ) { | ||
| final isDark = brightness == Brightness.dark; |
Address review feedback on the new colour system. Drop getAvailabilityColor's separate brightness parameter and read it from the ColorScheme instead, so a caller can no longer pass a dark scheme alongside Brightness.light. Nothing outside this PR calls it yet, so the signature is free to change now. Correct getAccentColor's doc comment: the unknown-category fallback is adapted for dark surfaces like any other hue, so the dark fallback is a lifted navy rather than the navy literal the comment claimed. No behaviour change; goldens unaffected.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
The app text theme bakes a colour into every style, so passing titleMedium/bodySmall straight into the AppBar painted near-black text over the navy bar, silently overriding AppBarTheme.foregroundColor. Measured against the bar's #2B3170: the title reached 1.45:1 and the subtitle 1.27:1, against WCAG AA's 4.5:1 minimum. Both now take the app bar's own foreground colour. My Festival is the only screen in lib/ that passes a text-theme style into an AppBar; every other screen inherits the bar's foreground and was already correct. Add a test that computes the real contrast ratio against the app bar's background and fails below 4.5:1. It pumps the actual app theme — the existing goldens use a stripped theme with no appBarTheme, which is why this never showed up in them.
The light-mode app bar was a solid navy slab, the only dark surface in an otherwise light UI. It now uses colorScheme.surface/onSurface in both themes, matching how dark mode already behaved and Material 3's default. The poster navy is unchanged as the seed and still leads through `primary`, the nav bar indicator and the category accents — only the app bar's own fill changes. Update app_theme_test to pin the new intent, and add a contrast check asserting the app bar title clears WCAG AA against its own background in both themes, so neither surface can drift into an unreadable pairing. No goldens changed: the screenshot tests build their own stripped theme with no appBarTheme, so they never covered the real app bar.
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://claude-ship-issues-469-ulciw.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #469
Consolidates the three ways colour is used as a signal — beverage category, stock availability, and personal tasted state — into
CategoryColorHelper, with the model documented in the class doc so future colour choices have one place to live. Two follow-on commits fix an app bar contrast failure found while reviewing the deployed preview.1. The colour system (
3206ddc)Removed dead
getCategoryColor(). The issue describes "two category-colour code paths that can drift". They were never both live —getCategoryColorhad zero call sites inlib/; the chips it was meant to serve usecolorScheme.primaryContainerdirectly. Deleting it removes the drift risk outright. Its ~260 lines of tests went with it.Added
getAvailabilityColor(). This duplication was real:_AvailabilityChip(drinks list) and_buildAvailabilityHint(My Festival) each hardcoded the same hex pairs independently. Now one accessor. Sold-out still resolves to the theme's semantic error colour rather than a fixed hex.Category accents now adapt to dark mode. The hue is kept exactly as-is for light mode and lightness-lifted for dark.
On the derivation
The obvious approach —
ColorScheme.fromSeed(seedColor: hue).primary— was measured and rejected. It desaturates the palette and collapses categories together:fromSeed().primaryA summed-RGB distance of 2 means perry and apple juice render as the same colour, which defeats the point of a per-category accent. The lift preserves hue and keeps separation roughly where it is today. A test pins a minimum-separation floor so a future derivation change can't quietly reintroduce a collision.
This is also simpler than the
fromSeedroute: it's a pure function, so no memoisation cache is needed despite being called per drink card in a scrolling list.2. API tightening (
b40532e)Review feedback.
getAvailabilityColorno longer takes a separatebrightness— it reads brightness from theColorScheme, so a dark scheme can't be paired withBrightness.light. Also corrects a doc comment that claimed the unknown-category fallback stays navy in dark mode, when it is lifted like any other hue.3. App bar contrast fix (
80a4418)buildAppTextThemebakescolorScheme.onSurfaceinto every style, so passingtitleMedium/bodySmallstraight into anAppBarsilently overridesAppBarTheme.foregroundColor. Against the navy bar this measured 1.45:1 for the title and 1.27:1 for the subtitle, versus WCAG AA's 4.5:1 minimum.My Festival was the only screen in
lib/doing this; every other screen inherits the bar's foreground and was already correct. Added a test that computes the real contrast ratio against the app bar background and fails below 4.5:1.4. Light app bar becomes a plain M3 surface (
30d6cd3)The light-mode app bar was a solid navy slab — the only dark surface in an otherwise light UI. It now uses
colorScheme.surface/onSurfacein both themes, matching how dark mode already behaved and Material 3's default. The poster navy is unchanged as the seed and still leads throughprimary, the nav bar indicator and the category accents.Goldens — and an honest gap
Light-mode accents are byte-identical by construction, so only the 6 dark goldens changed (of 15). Each was reviewed: accent-colour change only, no reflow.
The app bar change is not covered by any golden. All five golden-producing test files build their own bare
ThemeData(colorScheme: …)rather than callingbuildAppTheme(), so no golden has ever rendered the real app bar, nav bar, or text theme. That is precisely why the 1.27:1 subtitle survived in a repo with 15 goldens. Filed as #520. The app bar change is covered by theme-level contrast tests, but nothing renders it — the preview is the only visual check.Notes
getTastedColoris deliberately left independent ofgetAvailabilityColoreven though both are green today — personal status and stock level are separate signals that may diverge../bin/mise run checkgreen — analyzer clean, full suite passing.